Conversation
build.gradle
Outdated
|
|
||
| ext { | ||
| VERSION = "1.5.3" | ||
| VERSION = "1.5.4" |
There was a problem hiding this comment.
I think we should bump minor version here to 1.6.0.
| .whenComplete(new BiConsumer<ContextData, Throwable>() { | ||
| @Override | ||
| public void accept(ContextData contextData, Throwable throwable) { | ||
| if (throwable == null |
There was a problem hiding this comment.
What happens if throwable is not null?
There was a problem hiding this comment.
Nothing happens, just follow the stream calls and will fall in to the exceptionally. I'll check if the test validate this point, but I think yes.
There was a problem hiding this comment.
If throwable is null nothing happen in this code, the call will be sent to exceptionally step.
| if (contextData != null) | ||
| return contextData; | ||
|
|
||
| throw (CompletionException) throwable; |
There was a problem hiding this comment.
This cast is safe because all exceptions thrown the executions through CompletableFuture are wrapped in a CompletionException, right?
There was a problem hiding this comment.
The internal code always throw the exception with dataFuture.completeExceptionally. This will always return a CompletionException instance
|
|
||
| public void flushCache() { | ||
| List<PublishEvent> events = localCache.retrieveEvents(); | ||
| System.out.println("Sending events in cache: " + events.size()); |
There was a problem hiding this comment.
Yep deleted.
| import com.absmartly.sdk.json.PublishEvent; | ||
|
|
||
| public interface LocalCache { | ||
| void writeEvent(PublishEvent event); |
There was a problem hiding this comment.
Maybe call these writePublishEvent and retrievePublishEvents?
| private final ObjectMapper mapper; | ||
|
|
||
| public AbstractCache() { | ||
| final ObjectMapper objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
I think we should use the already existing interfaces here ContextEventSerializer and ContextDataSerializer.
The reason is that some users might not want to use Jackson, but use Gson or whatever else they may already have as a dependency, instead.
If we do this, then I think this abstract class is no longer necessary.
There was a problem hiding this comment.
@marcio-absmartly The problem is that we current don't have ContextDataSerializer and ContextEventDeserializer since we need to suport Serialize and Deserialize both. You think worth to create it only for this Cache purposes?
|
|
||
| private Connection getConnection() throws SQLException { | ||
| if (this.connection == null) { | ||
| this.connection = DriverManager.getConnection("jdbc:sqlite:absmarly.db"); |
There was a problem hiding this comment.
Should this file name be a parameter?
| implementation group: "com.fasterxml.jackson.datatype", name: "jackson-datatype-jsr310", version: jacksonDataTypeVersion | ||
| implementation group: 'org.xerial', name: 'sqlite-jdbc', version: sqliteVersion | ||
|
|
||
| implementation files('../libs/simpleCircuitBreaker-2.0.5.jar') |
There was a problem hiding this comment.
Are we going to distribute this in our repository?
Can we find something on maven-central or any other repository?
There was a problem hiding this comment.
Unfortunately, the developer has not released it to any Maven Repo :(
There was a problem hiding this comment.
The developer didn't answer that. Let keep this way until I get a response from them.
|
|
||
| public interface ContextEventHandler { | ||
| CompletableFuture<Void> publish(@Nonnull final Context context, @Nonnull final PublishEvent event); | ||
| void flushCache(); |
There was a problem hiding this comment.
This seems a bit out of place. The "cache" should be an implementation detail, right?
| final CompletableFuture<Void> future = readyFuture_; // cache here to avoid locking | ||
| if (future != null && !future.isDone()) { | ||
| future.join(); | ||
| future.whenComplete(new BiConsumer<Void, Throwable>() { |
There was a problem hiding this comment.
I don't think this should be here for three reasons:
- It seems like an implementation detail of the caching mechanism.
- What if the user never calls waitUntilReady? i.e. the user may create the context with an already retrieved
contextData. - It could be done independently when the
eventHandleris created, or instead, we could have a notification-like method in the interface, for examplecontextBecameReady()or something like that, that is called on theContextEventHandlerinterface when it gets ready (not just here).
There was a problem hiding this comment.
Did a change here, check if this place I put the code can have some side affect I not see.
2 participants
Implementing Resilience and Cache Layers in Java SDK